[download-microservice] Refactor: Clarify Execution, make bin finding clearer #132
Conversation
|
Just going to plunk down a link to a branch I was working on, as I hadn't see you were working on this! main...DeeDeeG:package-frontend:download-refactor-plus-more-error-handling I can have a look at what you've got here as well, but maybe I shouldn't have suggested changes, bopped offline, gone in a meditative trance editing these files a ton, watched some media, eaten a meal or two, and then come back online several hours later to see you've already done it yourself! |
|
The premise as per the PR description sounds really good, and kind of reaches all the things I wasn't bold enough to reach for as a contributor as opposed to original author. Probably gonna be pretty good, but I've been neck deep in the previous way of doing it for a few hours off and on, lol. So it'll take me a bit to absorb all these changes, but I think it's looking good at a first glance as well. |
|
@DeeDeeG I gotta say, I really love the changes you have on your branch. |
|
My favorite stuff in mine lives in Reading as I go, lol. |
| const url = req.url.split("?"); | ||
| const path = url[0]; | ||
| const queryParams = url[1]; |
There was a problem hiding this comment.
If you want to keep these const definitions and otherwise meld my changes of index.js into this PR it should apply pretty cleanly in terms of like diff conflicts and such.
Either this way or my branch are more readable than before, IMO. Your version here might be even slightly nicer, it's pretty subjective tho. path and queryParams seems very readable, IMO, as you have it on this branch already. 👍
There was a problem hiding this comment.
Maybe too much thoughts on this, tl;dr I think your var names are better.
I ended up with this on my branch (snippet), which I wasn't in love with but couldn't think of how to get exactly the names I wanted and the readability I wanted:
let [urlPath, urlParams] = req.url.split("?");The destructuring assignment on my branch, while concise, is IMO more confusing and less readable.
And not much point in reminding where these consts came from (urlPath and urlParams) by keeping "url" in their names, when path and queryParams are so descriptive, without fluff and readable.
So that's my thoughts why your variable names/assignment approach works better, IMO.
EDIT to add: I can draft a patch and offer it for your consideration to merge my favorite bits of the two versions of index.js if you haven't already done it?
EDIT 2: We discussed it on Discord. This became #133 targeting for inclusion in this PR's branch directly.
|
Also, I have a small thing on package-frontend/microservices/download/utils.js Lines 28 to 32 in 007e7a2 Would love to see something along those lines to be able to test the microservice without auth some way. UPDATE: Fixed in #133 which was merged into this PR's branch. |
Also fix un-authed API requests for dev mode
There was a problem hiding this comment.
Overall, with or without my changes, this bins.js thing and stringMatchesBin() separates things off a little more neatly than before, one might say.
It's less centralized, but I think each file is now more focused.
Also, things are labeled clearly with somewhat intuitive names, which I dunno about other folks, but that is a big one for me. Sometimes I come back to a project after weeks or months and it's like new code again for me. "Why was this set up like this? I forget..." Having very readable code in our "hopefully it just works" workhorse infrastructure is a great thing, IMO.
Even if this isn't the most dead simple way to possibly do it, it's one of the more understandable to read, when it comes down to it, and looks very "ergonomic" to use now, if working around in this code, and frankly I hope/expect it may not need much changes assuming we remain using GitHub Actions for the Rolling bins for the near future.
All that to say... looks good to me! 👍
microservices/download/utils.js
Outdated
There was a problem hiding this comment.
👍
I have to admit, the big diff makes this seem like a big change, but really it's pretty much splitting off the data about what the OSes/types are, and how to string manipulate the bin URL to verify it, off into bins.js and some logic to step through that to use the data in the new stringMatchesBin() function.
So as is so often the case with "move stuff around, reformat it" things (i.e. "refactoring," like the PR title, go figure)... we get a big diff for doing the same thing a different way.
No problem, and makes findLink() (the main user of stringMatchesBin()) a lot cleaner and shorter. And separates the stringMatchesBin() off as its own utility function, which I like. (Looks nicer just reading the new code in an editor, without the distraction of the large diff in the diff view.)
There was a problem hiding this comment.
Well, as mentioned on Discord, I'm requesting changes from myself, namely I committed as [email protected], which is not my proper commit email.
BRB while I fix that.
Noting that the PR branch is currently at commit 214adb2, which I have backed up at https://github.com/DeeDeeG/package-frontend/commits/PR-132-backup-before-rebase.
Sorry about that!
Neither risks much DDoS exposure, and this lets us update parameters in the future without having to remember this limit is here and update the limit as well. Prevents us from accidentally breaking the microservice, although the error messages if this got in our way would be extremely specific! But not much downside to increasing this a bit, either.
Also, give an error that's now a plain '500' a more specific message.
DeeDeeG's February 2024 refactor of the Download microservice (complementary to PR 132)
DeeDeeG
force-pushed
the
download/refactor
branch
from
214adb2
to
9ef6797
Compare
|
Woot, thanks for everything on this one @DeeDeeG |
This PR first addresses some great points @DeeDeeG brought up about code clarity, and aims to make things much more clear in behavior.
Secondly, since a refactor was already taking place, this PR removes likely the biggest portion of code. That is finding the bins. Previously this was a big long
if...elsewhich has no been replaced by a simple object, that allows easy lookup and definition of how to find each bin.The
./bin.jsfile exports an object where the top level key is the OS for each supported platform, which itself then has a top level key to eachtypeof supported installation on said platform. From there the following keys are supported to execute a check against the binary asset name:startsWith: Executesstring.startsWith(value)endsWith: Executesstring.endsWith(value)endsWithNot: Executes!string.endsWith(value)This way instead of repetitive code, with the definitions of how we find the binaries obscured within it, we instead have a single function decode this object into code with the clarity of the definitions being much more important.